Skip to content

Allow a step to be fatally failed via a RetryArtifact in its output data - #224

Merged
akashdw merged 3 commits into
mainfrom
ad/step-fatal-failure-signal
Jun 30, 2026
Merged

Allow a step to be fatally failed via a RetryArtifact in its output data#224
akashdw merged 3 commits into
mainfrom
ad/step-fatal-failure-signal

Conversation

@akashdw

@akashdw akashdw commented Jun 24, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Pull Request type

  • Bugfix
  • [x ] Feature
  • Refactoring (no functional changes, no api changes)
  • Build related changes (Please run ./gradlew build --write-locks to refresh dependencies)
  • Other (please describe):

NOTE: Please remember to run ./gradlew spotlessApply to fix any format violations.

Changes in this PR

  • Adds a RetryArtifact (maestro_retry) that a step can include in its output data with retryable: false; when such a step fails with a user or platform error, it is escalated to FATALLY_FAILED instead of
    being retried.
  • When the artifact is absent, or present with retryable: true, behavior is unchanged: the failure flows through the existing retry path.
  • This keeps failure classification inside Maestro via the existing output-data channel, no new model field or endpoint, and leaves room to extend the artifact with richer retry/classification controls
    later.

@akashdw akashdw changed the title Add non_retryable output data signal to fatally fail the step Allow a step to be fatally failed via a RetryArtifact in its output data Jun 26, 2026
rdeepak2002
rdeepak2002 reviewed Jun 29, 2026
View reviewed changes
Comment thread maestro-common/src/test/java/com/netflix/maestro/models/artifact/RetryArtifactTest.java
import com.netflix.maestro.MaestroBaseTest;
import org.junit.Test;

public class RetryArtifactTest extends MaestroBaseTest {

@rdeepak2002 rdeepak2002 Jun 29, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed that other tests in this module, such as WhileArtifactTest.java and TitusArtifactTest.java, include a testRoundTripSerde that verifies the general serde contract.

Would it make sense to add a similar testRoundTripSerde here for consistency and to cover the round-trip behavior as well?

rdeepak2002
rdeepak2002 approved these changes Jun 29, 2026
View reviewed changes

@rdeepak2002 rdeepak2002 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for creating this pr! Left a minor comment

@akashdw
akashdw merged commit d35dc9c into main Jun 30, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@derek-miller derek-miller derek-miller approved these changes
@rdeepak2002 rdeepak2002 rdeepak2002 approved these changes
@praneethy91 praneethy91 Awaiting requested review from praneethy91

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@akashdw @derek-miller @rdeepak2002